Skip to content

Typed Protocols: new API #1223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 21, 2024
Merged

Typed Protocols: new API #1223

merged 5 commits into from
Oct 21, 2024

Conversation

@coot
Copy link
Contributor

@coot coot commented Aug 21, 2024 •
edited
Loading

Description

Use typed-protocols-0.3.0.0.

Depends on:

  • Updated to use typed-protocols-0.3.0.0
  • Added KeepAlive tracer

@coot coot mentioned this pull request Aug 21, 2024
Closed
8 tasks
@coot coot self-assigned this Sep 27, 2024
@coot coot marked this pull request as ready for review September 27, 2024 05:21
@coot coot force-pushed the coot/typed-protocols-new-api branch 3 times, most recently from 9c13191 to a65b059 Compare September 28, 2024 08:49
nfrisby
nfrisby previously requested changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description seems a bit off.
image

  • It's version 0.3.0.0, not version 0.2.0.0.
  • I don't see any aspects of SizeInBytes within the diff itself.

The diff itself looked totally fine. I don't follow the new Stateful stuff yet, but everything here seems very reasonable.

I'm not Approving because of:

  1. The above concerns about the PR description.
  2. I don't think we want an source-repository-package stanza on the main branch. Will the relevant dependencies be uploaded to CHaP soon enough to wait for them?

@coot
Copy link
Contributor Author

coot commented Oct 1, 2024

I started with t-p-0.2 and ended up with t-p-0.3 😁. The TxSizeInBytes patch is gone from this PR, due to consensus introducing its own sizes in a recent PR.

@crocodile-dentist
Copy link
Contributor

ekg-forward 0.6 which is going into the next node release is incompatible with new typed-protocols 0.3 so lets hold off on merging this PR.

@coot
Copy link
Contributor Author

coot commented Oct 17, 2024

@crocodile-dentist ekg-forward is not a dependency of ouroboros-consensus; I have a draft PR for it as well (as for all other cardano-node dependencies 😃).

coot added 2 commits October 21, 2024 09:19
@coot
Updated to use typed-protocols-0.3.0.0
067dfc6
@coot
Added KeepAlive tracer
42a6bf1 
I also left TODO notes to add PeerSharing tracer.
@coot coot force-pushed the coot/typed-protocols-new-api branch 3 times, most recently from 518a441 to cb6fb68 Compare October 21, 2024 08:30
@coot
Updated to ouroboros-network-api-0.11
e8d3c09 
`NodeToClientV_19` was added.
@coot coot force-pushed the coot/typed-protocols-new-api branch from cb6fb68 to c4b3116 Compare October 21, 2024 08:47
@crocodile-dentist
Copy link
Contributor

@crocodile-dentist ekg-forward is not a dependency of ouroboros-consensus; I have a draft PR for it as well (as for all other cardano-node dependencies 😃).

Yes, but it would have caused problems downstream when releasing node - consensus would have to release off branch to make it go smoothest.

@coot coot force-pushed the coot/typed-protocols-new-api branch from c4b3116 to 254aa51 Compare October 21, 2024 11:21
@amesgen amesgen mentioned this pull request Oct 21, 2024
Merged
@amesgen amesgen dismissed nfrisby’s stale review October 21, 2024 15:38

The requested changes have been addressed.

@coot coot force-pushed the coot/typed-protocols-new-api branch from be7c0f2 to 120d92d Compare October 21, 2024 17:48
@coot coot added this pull request to the merge queue Oct 21, 2024
Merged via the queue into main with commit 7dd936f Oct 21, 2024
14 checks passed
@coot coot deleted the coot/typed-protocols-new-api branch October 21, 2024 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amesgen amesgen amesgen approved these changes

@nfrisby nfrisby nfrisby left review comments

@jasagredo jasagredo Awaiting requested review from jasagredo jasagredo is a code owner

@fraser-iohk fraser-iohk Awaiting requested review from fraser-iohk fraser-iohk is a code owner

@dnadales dnadales Awaiting requested review from dnadales dnadales is a code owner

Assignees

@coot coot

Labels

None yet

Projects

Ouroboros Network
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@coot @crocodile-dentist @nfrisby @amesgen